-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a WITH
clause
#6475
Conversation
Test fails seem to be related to changes to |
Can you please rebase this PR onto current master? It should fix build errors on Travis. 👍 |
@Majkl578 rebased, tests are green now ... but scrutinizer fails now. Yet the only issue now caused by my additions (https://scrutinizer-ci.com/g/doctrine/doctrine2/inspections/de206627-1966-4ae5-9710-e883754a9c58/issues/files/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php) is a false positive as |
lib/Doctrine/ORM/Query/SqlWalker.php
Outdated
* | ||
* @return string | ||
*/ | ||
public function walkRangeVariableDeclaration($rangeVariableDeclaration) | ||
public function walkRangeVariableDeclaration($rangeVariableDeclaration, $buildNestedJoins = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional parameter is problematic. The decision is one in the SqlWalker
anyway, so the correct solution (IMO) would be to move the method body to a private method, then keep the public API as-is and delegate the call on the public API to the private API, where the new parameter is defined.
/** @Column(type="integer") */ | ||
public $authorId; | ||
|
||
/** @Column(length=100) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop any details that aren't strictly needed. If columns aren't affected, they shouldn't be in here
public function testIssue() | ||
{ | ||
|
||
$qb = $this->_em->createQueryBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please either inline with the code below or use a DQL string instead of the builder
@@ -891,7 +892,11 @@ public function walkRangeVariableDeclaration($rangeVariableDeclaration) | |||
); | |||
|
|||
if ($class->isInheritanceTypeJoined()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this block be moved to a private method too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt that aids readability since the decision is made by the caller (actually even that one's caller) and not the extracted function itself ... and it just modifies local state (i.e. $sql
).
lib/Doctrine/ORM/Query/SqlWalker.php
Outdated
@@ -1137,11 +1142,11 @@ public function walkJoin($join) | |||
$conditions[] = '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')'; | |||
} | |||
|
|||
$condExprConjunction = ($class->isInheritanceTypeJoined() && $joinType != AST\Join::JOIN_TYPE_LEFT && $joinType != AST\Join::JOIN_TYPE_LEFTOUTER) | |||
$condExprConjunction = ($class->isInheritanceTypeJoined() && $joinType != AST\Join::JOIN_TYPE_LEFT && $joinType != AST\Join::JOIN_TYPE_LEFTOUTER && empty($conditions)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty($conditions)
is used twice - can you give it a name and assign it to a meaningful variable?
WITH
clause
public function testIssue() | ||
{ | ||
$query = $this->_em->createQueryBuilder() | ||
->select('p', 'a') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of select
is invalid - removing a
from the selection, as you should NEVER fetch-join results over an association that was filtered.
Why this issue is not fixed yet? For our new project we use Doctrine 2 and MSSQL and with first join query we encountered the same problem. When I dump the sql build from Doctrine it wraps the joined table name with parenthesis "JOIN (table tb) ON...." which gives error for MSSQL In our case, we have Users table which should be joined to the table we are quering. The User entity is softedeletable, which adds condition in the join clause. But becouse of this condition the SqlWalker wraps the table users with parenthesis here:1084
|
See #6464 for a detailed example of the problem.
For a query like
... where
GH6464Author
is a CTI entity, the sql generation currently generates two JOINs (correct) and attaches both join conditions to the latter JOIN. This works with MySQL and SQLite, however fails on PostgreSQL.This patch
SqlWalker
that makes it output a nested JOINSelectSqlGenerationTest
to reflect the change (and make those SQL statements valid -- on PostgreSQL)